feat: add Git server over HTTP & SSH and fix protocol handling#61
Conversation
Signed-off-by: jackieismpc <jackieismpc@gmail.com>
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR fixes critical protocol-layer bugs in packet-line handling and introduces side-band multiplexing support for Git smart protocol operations, along with complete HTTP and SSH server examples that demonstrate interoperability with standard Git clients.
Key changes:
- Fixed
read_pkt_lineto use non-consumingslice()instead ofcopy_to_bytes()for length parsing, preventing premature buffer consumption on incomplete packets - Added proper side-band packet framing for upload-pack and receive-pack operations when clients negotiate side-band capabilities
- Enhanced receive-pack stream parsing to validate flush markers and properly separate ref commands from pack data
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/protocol/utils.rs | Fixed pkt-line reading to avoid consuming buffer on length parsing; added advance() calls at correct positions; added test for incomplete packet handling |
| src/protocol/smart.rs | Cleared capabilities/state on each operation; removed duplicate ACK lines; enhanced receive-pack stream parsing with flush validation and capability extraction from first line |
| src/protocol/core.rs | Added side-band packet wrapping for upload-pack and receive-pack; implemented build_side_band_packets helper; added comprehensive side-band tests |
| Examples/http_server.rs | Full HTTP Git server using Axum framework with filesystem-backed repository implementation; handles info/refs, upload-pack, and receive-pack endpoints |
| Examples/ssh_server.rs | SSH Git server that reads SSH_ORIGINAL_COMMAND, handles upload-pack/receive-pack over stdin/stdout; includes robust path traversal protection |
| Examples/encode_pack.rs | Removed trailing whitespace from documentation comment |
| let length = payload.len() + 5; // 4-byte length + 1-byte band | ||
| let mut pkt = BytesMut::with_capacity(length); | ||
| pkt.put(Bytes::from(format!("{length:04x}"))); |
There was a problem hiding this comment.
The side-band packet length calculation includes the 4-byte hex length prefix in the total length field (line 357), but then formats the length as hex and prepends it (line 359). This means the length field itself counts toward the total, which is correct for Git pkt-line format. However, the calculation should ensure that when max_payload is at the boundary (e.g., 65515 for side-band-64k), the total packet length (payload + 5) doesn't exceed 65520. The current code could theoretically create a packet of length 65520 (if payload = 65515), resulting in a 4-byte hex length of "fff0", which is valid. Please verify this boundary condition is handled correctly per Git protocol specification.
| #[derive(Clone)] | ||
| struct FsRepository { | ||
| git_dir: Arc<PathBuf>, | ||
| } | ||
|
|
||
| impl FsRepository { | ||
| /// Create a new FsRepository for the given git directory. | ||
| fn new(git_dir: PathBuf) -> Self { | ||
| Self { | ||
| git_dir: Arc::new(git_dir), | ||
| } | ||
| } | ||
|
|
||
| /// Create a git command with the appropriate git-dir. | ||
| fn git_cmd(&self) -> Command { | ||
| let mut cmd = Command::new("git"); | ||
| cmd.arg("--git-dir").arg(&*self.git_dir); | ||
| cmd | ||
| } | ||
|
|
||
| /// Run a git command with the given arguments. | ||
| async fn run_git<I, S>(&self, args: I) -> Result<std::process::Output, ProtocolError> | ||
| where | ||
| I: IntoIterator<Item = S>, | ||
| S: AsRef<std::ffi::OsStr>, | ||
| { | ||
| self.git_cmd() | ||
| .args(args) | ||
| .output() | ||
| .await | ||
| .map_err(ProtocolError::Io) | ||
| } | ||
|
|
||
| /// Get the path to the objects directory. | ||
| fn objects_dir(&self) -> PathBuf { | ||
| self.git_dir.join("objects") | ||
| } | ||
|
|
||
| /// Write a loose object to the objects directory. | ||
| fn write_loose_object( | ||
| &self, | ||
| obj_type: ObjectType, | ||
| data: &[u8], | ||
| ) -> Result<ObjectHash, ProtocolError> { | ||
| let hash = ObjectHash::from_type_and_data(obj_type, data); | ||
| let hex = hash.to_string(); | ||
| let (dir, file) = hex.split_at(2); | ||
| let obj_dir = self.objects_dir().join(dir); | ||
| let obj_path = obj_dir.join(file); | ||
|
|
||
| if obj_path.exists() { | ||
| return Ok(hash); | ||
| } | ||
|
|
||
| std::fs::create_dir_all(&obj_dir).map_err(ProtocolError::Io)?; | ||
|
|
||
| let mut encoder = ZlibEncoder::new(Vec::new(), Compression::default()); | ||
| let header = format!("{obj_type} {}\0", data.len()); | ||
| encoder | ||
| .write_all(header.as_bytes()) | ||
| .map_err(ProtocolError::Io)?; | ||
| encoder.write_all(data).map_err(ProtocolError::Io)?; | ||
| let compressed = encoder.finish().map_err(ProtocolError::Io)?; | ||
|
|
||
| std::fs::write(&obj_path, compressed).map_err(ProtocolError::Io)?; | ||
| Ok(hash) | ||
| } | ||
|
|
||
| /// Parse a raw tree listing into TreeItems. | ||
| fn parse_tree_listing(&self, raw: &[u8]) -> Result<Vec<TreeItem>, ProtocolError> { | ||
| let mut items = Vec::new(); | ||
| let text = String::from_utf8_lossy(raw); | ||
| for line in text.lines() { | ||
| if line.is_empty() { | ||
| continue; | ||
| } | ||
| let (meta, name) = line | ||
| .split_once('\t') | ||
| .ok_or_else(|| ProtocolError::invalid_request("Invalid tree entry"))?; | ||
| let mut parts = meta.split_whitespace(); | ||
| let mode_raw = parts | ||
| .next() | ||
| .ok_or_else(|| ProtocolError::invalid_request("Missing tree mode"))?; | ||
| let _kind = parts | ||
| .next() | ||
| .ok_or_else(|| ProtocolError::invalid_request("Missing tree kind"))?; | ||
| let hash_str = parts | ||
| .next() | ||
| .ok_or_else(|| ProtocolError::invalid_request("Missing tree hash"))?; | ||
|
|
||
| let mode_norm = mode_raw.trim_start_matches('0'); | ||
| let mode_bytes = if mode_norm.is_empty() { | ||
| b"0" | ||
| } else { | ||
| mode_norm.as_bytes() | ||
| }; | ||
| let mode = TreeItemMode::tree_item_type_from_bytes(mode_bytes) | ||
| .map_err(|e| ProtocolError::repository_error(e.to_string()))?; | ||
| let id = | ||
| ObjectHash::from_str(hash_str).map_err(|e| ProtocolError::repository_error(e))?; | ||
|
|
||
| items.push(TreeItem::new(mode, id, name.to_string())); | ||
| } | ||
| Ok(items) | ||
| } | ||
| } |
There was a problem hiding this comment.
The FsRepository implementation (lines 84-189) is duplicated between http_server.rs and ssh_server.rs with identical logic. This violates DRY (Don't Repeat Yourself) principles and makes maintenance harder. Consider extracting this shared repository implementation into a separate module or into the library itself so both examples can reuse it.
| /// Auth | ||
| #[derive(Clone)] | ||
| struct AllowAllAuth; | ||
|
|
||
| #[async_trait] | ||
| impl AuthenticationService for AllowAllAuth { | ||
| async fn authenticate_http( | ||
| &self, | ||
| _headers: &HashMap<String, String>, | ||
| ) -> Result<(), ProtocolError> { | ||
| Ok(()) | ||
| } | ||
|
|
||
| async fn authenticate_ssh( | ||
| &self, | ||
| _username: &str, | ||
| _public_key: &[u8], | ||
| ) -> Result<(), ProtocolError> { | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
The AllowAllAuth implementation (lines 360-380) is duplicated between http_server.rs and ssh_server.rs with identical logic. Consider extracting this into a shared test/example utilities module to avoid duplication.
| fn read_pkt_line_incomplete_does_not_consume() { | ||
| let mut buf = Bytes::from_static(b"0009do"); | ||
| let before = buf.len(); | ||
| let (len, data) = read_pkt_line(&mut buf); | ||
| assert_eq!(len, 0); | ||
| assert!(data.is_empty()); | ||
| assert_eq!(buf.len(), before); | ||
| } |
There was a problem hiding this comment.
The test checks that an incomplete packet (6 bytes when 9 are needed) doesn't consume input, which is good. However, the test doesn't verify the actual return value semantics. Consider adding an assertion that the returned data is empty and len is 0, which would make the expected behavior more explicit and catch regressions if the implementation accidentally returns partial data.
|
|
||
| fn resolve_repo_path(repo_root: &StdPath, repo: &str) -> Result<PathBuf, Response> { | ||
| // Reject traversal and map repo name to bare or non-bare layout. | ||
| if repo.is_empty() || repo.contains("..") || repo.contains('\\') || repo.contains('/') { |
There was a problem hiding this comment.
The path traversal check only rejects strings containing ".." directly, but does not properly validate against path components. An attacker could still pass a repo name like "demo.git/../../../../etc" if the "/" check didn't exist. While the "/" check at line 510 helps, the ".." check alone is insufficient because it only checks if ".." appears as a substring anywhere in the string, not as a path component. Consider using a more robust approach that validates each path component explicitly (similar to the SSH server implementation which uses Component::ParentDir check).
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jackie <jackieismpc@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jackie <jackieismpc@gmail.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(pack_data) = pack_data { | ||
| // Create pack generator for unpacking | ||
| let pack_generator = PackGenerator::new(&self.repo_storage); | ||
| // Unpack the received data | ||
| let (commits, trees, blobs) = pack_generator.unpack_stream(pack_data).await?; |
There was a problem hiding this comment.
If the client sends ref commands followed by a flush without any packfile bytes (e.g., a crafted push with new_hash set to a non‑zero object id but no pack data), the code skips unpacking in this block and still proceeds to return "unpack ok" and execute update_reference for every command later in the function. Without validating that the referenced objects actually exist, a client can advance refs to missing objects when no packfile was sent, leaving the repository with dangling refs. Consider rejecting non‑delete updates when pack_data is absent or verifying the new objects via has_object before updating refs.
Useful? React with 👍 / 👎.
…-tools#61) * fix protocol Signed-off-by: jackieismpc <jackieismpc@gmail.com> * add examples for ssh and http * fmt fix * Update Examples/ssh_server.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jackie <jackieismpc@gmail.com> * Update Examples/ssh_server.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jackie <jackieismpc@gmail.com> --------- Signed-off-by: jackieismpc <jackieismpc@gmail.com> Signed-off-by: Jackie <jackieismpc@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary
This PR fixes the protocol-layer implementation to enable correct interoperability with standard Git clients, and introduces Git servers implementation that supports HTTP and SSH transports for local end-to-end testing and reference usage.
Key changes
•Fixed protocol handling to ensure standard Git client interaction works as expected (handshake/capabilities/service flow).
•Added a lightweight Git server example/implementation for HTTP transport.
•Added a lightweight Git server example/implementation for SSH transport.
#52